-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(startWith): remove deprecated scheduler signature. #7166
feat(startWith): remove deprecated scheduler signature. #7166
Conversation
7098f11
to
7b2a806
Compare
src/internal/operators/startWith.ts
Outdated
// Devs are more likely to pass null or undefined than they are a scheduler | ||
// without accompanying values. To make things easier for (naughty) devs who |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After the scheduler was removed, these comments became useless. Maybe we should remove them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are signatures with null
and undefined
still relevant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think they aren't relevant anymore.
src/internal/observable/innerFrom.ts
Outdated
for (let i = 0; i < length && !subscriber.closed; i++) { | ||
subscriber.next(array[i]); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we exit the function as soon as subscriber.closed
becomes true
. Does it make sense to do something similar here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is covered in the for
loop. This part: && !subscriber.closed
should ensure that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just that in example, when subscriber.closed
becomes true
, then subscriber.complete()
is not called, but in subscribeToArray
it is called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If subscriber.complete()
gets called, but subscription is already closed, it won't make any difference. Subscriber will not get complete
notification multiple times, so this is OK I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I know that complete
won't be notified several times. I just wanted to ask and suggest doing something like this:
for (let i = 0; i < length && !subscriber.closed; i++) { | |
subscriber.next(array[i]); | |
} | |
for (let i = 0; i < length; i++) { | |
if (subscriber.closed) { | |
return; | |
} | |
subscriber.next(array[i]); | |
} |
Nevermind, it's just my curiosity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for (let i = 0; i < length && !subscriber.closed; i++) { | |
subscriber.next(array[i]); | |
} | |
for (let i = 0; i < length; i++) { | |
if (subscriber.closed) { | |
return; | |
} | |
subscriber.next(array[i]); | |
} |
I double-checked and made sure that if you do not return inside the loop, then onStoppedNotification
will be called.
config.onStoppedNotification = (notification) => {
console.log('onStoppedNotification', notification);
};
EMPTY.pipe(startWith(1, 2, 3), take(2)).subscribe();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is mildly more efficient, I suppose. A conditional followed by a return, rather than a conditional followed by a function call, a conditional and a return. It's not likely to make any performance difference though, I'd guess. Even at high-speed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are talking about a false positive onStoppedNotification
call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The difference is that return
prevents subscriber.complete()
.
const expected = 's--a--b--- '; | ||
const values = { s: 's', a: 'a', b: 'b' }; | ||
|
||
const result = e1.pipe(startWith('s', testScheduler)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a reinsurance. Are you sure that these tests should be deleted? Isn't it enough just to remove testScheduler
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, they're invalid tests now. This would test that it would start with 's'
and then an instance of testScheduler
. :)
src/internal/observable/innerFrom.ts
Outdated
for (let i = 0; i < length && !subscriber.closed; i++) { | ||
subscriber.next(array[i]); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for (let i = 0; i < length && !subscriber.closed; i++) { | |
subscriber.next(array[i]); | |
} | |
for (let i = 0; i < length; i++) { | |
if (subscriber.closed) { | |
return; | |
} | |
subscriber.next(array[i]); | |
} |
I double-checked and made sure that if you do not return inside the loop, then onStoppedNotification
will be called.
config.onStoppedNotification = (notification) => {
console.log('onStoppedNotification', notification);
};
EMPTY.pipe(startWith(1, 2, 3), take(2)).subscribe();
- Removes deprecated signature with scheduler + Slightly improves performance by allocating fewer objects during subscription BREAKING CHANGE: `startWith` no longer accepts a scheduler. If you would like to schedule emissions in this manner, instead of `source$.pipe(startWith(1, 2, 3, scheduler)) use `concat(scheduled([1, 2, 3], scheduler), source$)`.
…scourage its use
95f8d95
to
13e19fc
Compare
subscribeToArray
closer to usage.